fix(plugin-postgresql): follow pg_dump's index rule so invalid indexes stay out of dumps and copies - #3075
Merged
Conversation
…currently, and gate its structure edits by kind
…s stay out of dumps and copies
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
…index-ddl # Conflicts: # TablePro/Resources/Localizable.xcstrings # docs/databases/postgresql.mdx
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #3063
Summary
A PostgreSQL index left invalid by a failed or cancelled
CREATE INDEX CONCURRENTLYorREINDEX CONCURRENTLYwas written into SQL exports, the DDL tab, MCPget_table_ddland the column reorder script as an ordinaryCREATE INDEX, so replaying any of them failed on the duplicate rows the build had tripped over and rolled back. Compare & Sync and Copy To recreated it on the target. The same read dropped a plain unique index that a foreign key depends on, and exclusion constraints were in neither the table DDL nor the index DDL.This PR gives PostgreSQL one index inclusion rule, pg_dump's own, and uses it everywhere a copy of a table is written. The Indexes tab keeps the row, because the index is still maintained on every write and still refuses duplicates, and names it in a line under the grid.
Root cause
Two hand-written queries decided which indexes stand outside a table's constraints, and neither implemented pg_dump's
getIndexesrule.fetchIndexDDLhad noindisvalidorindisreadypredicate, and it excluded an index when anypg_constraintrow named it inconindid. A foreign key'sconindidnames the unique index it references, so that index was dropped from the dump. pg_dump joins onconrelid = indrelid AND conindid = indexrelid AND contype IN ('p','u','x').pg_indexesand excluded indexes whose name matched any constraint on the table, so it kept invalid indexes and dropped a valid index that shared a name with a CHECK constraint.fetchTableDDLwrotecontype IN ('p','u','c'), so anEXCLUDEconstraint was in neither half.PluginIndexInfocarried no validity, so the grid, Compare, Object Copy and MCP could not tell an invalid index from a working one.Two ordering defects made the foreign-key case fail even with the index present: the SQL export wrote every deferred
ADD FOREIGN KEYbefore the index phase, and the reorder script re-added foreign keys before the indexes. pg_dump writes indexes first.What changed
PostgreSQLIndexQueries.restorableIndexPredicateis pg_dump REL_17's rule,(ix.indisvalid OR t.relkind = 'p') AND ix.indisready.standaloneIndexQueryprojects it besidepg_get_indexdefand uses pg_dump's constraint join;standaloneIndexes(rows:)splits restorable definitions from invalid names.fetchIndexDDL(export, DDL tab, Copy DDL, MCPget_table_ddl, PGlite) and column reorder both read through it.indexListprojects the same predicate asis_valid, so the grid note, Compare, Object Copy, MCP and the export agree on one rule. AnON ONLYindex on a partitioned table waiting for a partition's index to be attached is kept by pg_dump, so it reads as valid here too and gets no note.PluginIndexInfo.isValid: Bool?arrives through a new full initializer; the previous full initializer is now@_disfavoredOverloadand unchanged. nil means the driver does not report it and reads as valid. SynthesizedCodable, so a payload without the key decodes to nil. Kit version stays at the pending 33, with a note beside the others.IndexInfo.isValid(default true) carries it.TableStructureRead.sourceSnapshotdrops invalid indexes; Compare's source side, the data compare's source side and Object Copy's source read use it, while the target keeps its invalid indexes. So an invalid source index is never created on a target, a target's invalid twin of a source index is left alone rather than re-added into an "already exists", and an orphan invalid index on the target is still offered for dropping.PostgreSQLTableRebuild(pure, in the test target) so it can be tested; it also names the invalid indexes it leaves out in a caveat.PostgreSQLSchemaQueries.tableDDLConstraintsQuery(moved out offetchTableDDL) adds'x'.InvalidIndexNoteover the concurrent-refresh note.ConcurrentRefreshNoteViewbecameStructureTabNoteView(systemImage:text:identifier:); the refresh note keepsstructure-concurrent-refresh-note, the new note isstructure-invalid-index-note.is_valid, declared (required) inMCPToolSchema.indexDefinitionand indocs/external-api/mcp-tools.mdx.scripts/check-postgres-index-dump-parity.shbuilds every shape below on a live server and diffs both the index names and the constraint names againstpg_dump -s, after grepping the Swift predicates out of the source.Measured
PostgreSQL 17.11 and pg_dump 17.11, throwaway server on 127.0.0.1:54329.
Shapes:
twith a failed unique CIC over duplicates (t_email_key, valid f / ready f), a unique CIC cancelled while waiting out an older snapshot (t_code_key, f/t), a cancelledREINDEX CONCURRENTLY(t_code_idx_ccnew, f/t);parent_code_idx, a plain unique indexchild_code_fkeyreferences;s_v_idx, an index sharing its name with a CHECK constraint;ex, anEXCLUDE USING gist; a partitionedpwith anON ONLYindex attached on one of two partitions.scripts/check-postgres-index-dump-parity.sh:tindexest_code_idx t_code_idx_ccnew t_code_key t_email_key, pg_dumpt_code_idxparentindexesparent_code_idxexconstraintsex_r_exclThe real driver, built with
swiftcagainst the worktree'sTableProPluginKit.frameworkand run against the same schema, base branch then this branch:fetchIndexDDL(t)t_code_idx,t_code_key,t_email_keyt_code_idxfetchIndexDDL(parent)parent_code_idxfetchTableDDL(ex)r int4rangeonlyEXCLUDE USING gist (r WITH &&)t, run in a transactioncould not create unique index "t_email_key",Key (email)=(a@x) is duplicatedparent, run in a transactionthere is no unique constraint matching given keys for referenced table "parent"ss_v_idxnever recreatedfetchIndexes(t)isValidt_code_keyandt_email_keyfalse, others truefetchIndexes(p)isValidp_a_idx(ON ONLY, waiting) true, as pg_dump keeps itReplaying the export's old order by hand:
ALTER TABLE child ADD CONSTRAINT child_code_fkey ... REFERENCES parent (code)beforeCREATE UNIQUE INDEX parent_code_idxfails withthere is no unique constraint matching given keys for referenced table "parent"; the new order runs.PluginKit ABI:
scripts/check-pluginkit-abi.shagainst the merge base withorigin/main: additive only. One stored property (isValid: Bool?) and one initializer are added, and the previous full initializer gains@_disfavoredOverloadwith its signature unchanged; nothing is removed.nmon the built framework still exports the 7-argument and the 11-argument initializers beside the new 12-argument one.currentPluginKitVersionwas already raised to 33 this cycle (v0.75.0 ships 32), so it is reused and noInfo.plistchanges.Tests
verify.sh teston every suite that owns a changed type, after the final edit: 356 executed, 356 passed.New suites:
PostgreSQLStandaloneIndexQueryTests(5),PostgreSQLTableDDLConstraintsQueryTests(1),PostgreSQLTableRebuildTests(5),TableStructureSnapshotIndexValidityTests(5),InvalidIndexNoteTests(3). New cases in existing suites:PostgreSQLIndexKeyPartTests(3),PluginIndexInfoCodableTests(extended),PluginIndexMappingCoverageTests(1),MCPIndexEncodingTests(2),SQLExportIndexPhaseTests(1, replacing a case whose name claimed indexes follow the foreign keys).PluginStructureFixturesnow variesisValid, soPluginStructureMappingTestscatches a mapper that drops it. The new builders are registered inPostgreSQLLegacyCatalogQueryTests(9.1 portability and hostile names) andPostgreSQLLiteralQuotingTests.Also:
verify.sh build(TablePro, PostgreSQLDriver, SQLExport) pass;verify.sh docspass;verify.sh linton every changed Swift file shows no violation on a changed line (two pre-existingpublic_error_text_in_loghits atPostgreSQLPluginDriver.swift:812and:837are untouched lines; the pre-existingfunction_body_lengthonfetchRebuildPartsis fixed by moving its column read out);shellcheck --severity=warningon the new script is clean.Each new test was run red by undoing its fix in one pass (all mutations applied together, then restored):
SQLExportIndexPhaseTests.indexesPrecedeDeferredForeignKeysPostgreSQLTableRebuildTests.indexesPrecedeForeignKeysPostgreSQLTableRebuildTests.invalidIndexesAreNamedsourceSnapshotstops filteringTableStructureSnapshotIndexValidityTestssourceSnapshotDropsInvalidIndexes,invalidSourceIndexIsNotSynced,objectCopyLeavesInvalidIndexOutIndexInfo(_:)mapsisValidto a constantPluginStructureMappingTests.indexCarriesEveryFieldis_validMCPIndexEncodingTests.validityIsEncodedix.indisreadyPostgreSQLStandaloneIndexQueryTests.restorableFollowsPgDump,PostgreSQLIndexKeyPartTests.validityUsesTheDumpRule'x'removed from the table DDL constraintsPostgreSQLTableDDLConstraintsQueryTests.exclusionConstraintsAreIncludedInvalidIndexNoteTestsnamesOnlyTheInvalidIndex,listsEveryInvalidIndex13 cases red, all expected, and green again after restoring.
targetInvalidTwinIsNotReAddedandorphanTargetInvalidIndexIsDroppedguard the other direction: they go red if the target side is filtered too.No UI test: an invalid index needs a live PostgreSQL server and a concurrent build made to fail, which the UI suite does not provide.
Before / After
Screenshots to be added. States to capture, light and dark:
t_code_key): the grid lists it and the line under the grid reads "t_code_key is invalid, so queries skip it and exports leave it out. Drop it, or rebuild it with REINDEX."t_code_key,t_email_key): the plural line.t: before listsCREATE UNIQUE INDEX t_code_keyandt_email_key, after does not.Critique points not taken
None. Every objection in the review was applied:
'x'in the table DDL constraint read, and the parity script diffs constraints as well as indexes.ON ONLYpartitioned index waiting onATTACH: the predicate reads it as valid, so the note leaves it out.isValidisBool?through a new overload with synthesizedCodable.is_validdeclared in the MCP schema and docs; the new builders registered in the 9.1-portability and hostile-name suites.Deviation from the design: the design projected raw
ix.indisvalid. This projects pg_dump's rule instead, so one predicate decides the note, Compare, Object Copy, MCP and the export, and the partitioned case needs no table kind in the app (a structure tab'sobjectKindfalls back to.tablewhen the tab was opened without one, which would have worded the partitioned case wrongly).Deliberately not fixed here
ALTER INDEX ... ATTACH PARTITIONis ever written, so a partitioned table'sON ONLYindex restores invalid even when the source's was valid.Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift:453(fetchIndexDDL) and the export's index phase would need the attach statements pg_dump writes.pg_get_constraintdefreturns the body alone: a primary key, unique, check or exclusion constraint restores under PostgreSQL's default name, and a standalone index already holding that default name then fails the restore.Plugins/PostgreSQLDriverPlugin/PostgreSQLSchemaQueries.swift:478.EditableIndexDefinitionand a DROP plus CREATE in the script.TablePro/Core/Compare/CompareRunner.swift:305.is_disabled, OracleUNUSABLE, MySQLINVISIBLE. Each driver's index read would setisValid.fetchIndexDDLreturns[String], so a note would need the export data source to carry the skipped names.